-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3, 4단계 - 체스] 우르(김현우) 미션 제출합니다. #539
Conversation
- PositionCache 를 통해서 가능한 위치들을 애플리케이션이 올라갈 때 미리 캐싱을 해둠
- Score equals 코드 수정
@@ -0,0 +1,26 @@ | |||
package chess.dao; | |||
|
|||
public class BoardModifyDao { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 DAO 와 Repository 의 개념을 제대로 이해하지 못한 것 같습니다,, 😢
DAO와 Repository는 리뷰 받고 나서 바로 수정하도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우르 3,4단계 굉장히 빠르고 명쾌하게 해주셨네요. 상세한 코멘트 재밌게 잘 읽었습니다.
크루들이 말한 패턴들을 읽어보고 조금 적용해보니, 코드 수는 줄일 수 있더라도 반비례로 상대방이 더 이해하기 어렵다고 생각했어요.
아주 공감합니다. 패턴에 목매면 안된다고 생각합니다. 본질은 결국엔 좋은 코드고 가독성은 좋은 코드의 큰 요소 중 하나이니까요!
코멘트 달아놨으니 확인해주시면 감사하겠습니다.
public static boolean isStatus(final String command) { | ||
return Arrays.stream(values()) | ||
.filter(it -> it == STATUS) | ||
.anyMatch(it -> it.value.equals(command)); | ||
} | ||
|
||
public static boolean isNewGame(final String command) { | ||
return Arrays.stream(values()) | ||
.filter(it -> it == NEW_GAME) | ||
.anyMatch(it -> it.value.equals(command)); | ||
} | ||
|
||
public static boolean isLoadGame(final String command) { | ||
return Arrays.stream(values()) | ||
.filter(it -> it == LOAD_GAME) | ||
.anyMatch(it -> it.value.equals(command)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values()를 스트림 돌려야되는 다른 이유가 있나요?? 그냥 바로 STATUS, LOAD_GAME, NEW_GAME 의 value랑 equals 비교만 해주면 안되나요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇네요,, 굳이 위에 있는End, Move, NotStart 도 스트림을 돌릴 필요가 없을 것 같아요
} | ||
|
||
public static String mapToPieceInitialFrom(final Piece piece) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final BoardRegisterDao boardRegisterDao = | ||
new BoardRegisterDao(position, | ||
boardRegisterRequest.turn().name() | ||
); | ||
|
||
return boardRepository.save(boardRegisterDao); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto 개념으로 사용하셨군요.
repository로 무언가를 요청할 때에 save와 같은 기본적인 기능들은 굳이 dto로 안넘기고 도메인 그 자체나 도메인 요소들로 넘겨도 크게 상관이 없습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵,, DAO를 DTO 형제라고 생각하고 단지 역할이 다르다고만 생각했습니다. 하지만 자세히 찾아보니 DAO에 통용된 개념이 있더라구요.
그래서 수정했습니다!!
repository로 무언가를 요청할 때에 save와 같은 기본적인 기능들은 굳이 dto로 안넘기고 도메인 그 자체나 도메인 요소들로 넘겨도 크게 상관이 없습니다.
파즈가 말씀해주신 이 부분에 대해서 궁금한 점이 있어요.
도메인을 그대로 넘긴다는 말씀은 만약 엔티티의 타입과 DB 데이터의 타입이 다르다면 mapping을 DAO에서 해주신다는 말씀이실까요?
현재 체스에서는 자바에서는 Map, DB에서는 String 이므로 따로 맵핑될 필요가 있다고 생각했습니다. 그래서 DAO 는 데이터를 영속화 시켜주는 무거운 일을 하기 때문에 이 일만 하는 것이 적절하다고 생각이 들어서 서로 다른 layer 간의 오케스트레이션 역할을 하는 Service에서 해주려고 합니다.
파즈는 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입이 다르다는 것을 제가 간과했네요~~. 우르의 말도 맞는 말입니다. 그렇다면 현재 BoardDao
클래스 내부의 mappingToDtoFrom
또한 매핑의 일종인데 이 부분은 어떻게 생각하시나요?
사용자 입장에서는 Board
도메인을 DB에 저장하고 싶고, 단순히 BoardDao
를 이용해서 save 하고 싶은데 어떤 식으로 mapping을 해야하는지 service layer에 두게된다면 service layer에 DB 저장에 필요한 변환에 대한 역할을 맡게되는 것인데 이는 괜찮다고 생각하시나요? 정답은 없으니 자유롭게 의견 말씀해주시면 감사하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇다면 현재 BoardDao 클래스 내부의 mappingToDtoFrom 또한 매핑의 일종인데 이 부분은 어떻게 생각하시나요?
mappingToDtoFrom
은 현재 DB에서 반환된 ResultSet을 DTO로 맵핑해주는 역할을 하고 있습니다. 만약에 DTO가 아닌 Domain을 그대로 반환해준다고 해도 결국 이러한 로직은 DAO에 존재할 것 같습니다.
왜냐하면 파즈의 말씀대로 도메인을 반환하고 싶다면, ResultSet
에서도 결국 Domain에 대한 mapping을 해줘야하기 때문에 반환된 타입만 다르지 같은 로직이라고 생각합니다.
resultSet.getLong("BOARD_ID"),
resultSet.getString("POSITION"),
resultSet.getString("TURN")
위와 같은 로직은 ResultSet
즉, java.sql 패키지에 존재하기 때문에 sql 관련 의존성은 DAO에서 가지고 있는게 적합하다고 생각해요.
service layer에 DB 저장에 필요한 변환에 대한 역할을 맡게되는 것인데 이는 괜찮다고 생각하시나요?
넵 저의 짧은 경험으로 설명드리자면 DAO 자체는 현재 DB에 접근하는 아주 무거운 역할을 하고 있다고 생각합니다.
그래서 애플리케이션 데이터 타입과 DB의 데이터 타입 불일치를 해결하는 곳이 DAO보다는 여러 layer 간의 상호작용을 도와주는 service layer에서 해주는게 적절하다고 생각합니다.
이것 또한 하나의 비즈니스 로직이라고 생각들구요.
그렇다면 DB로부터 값을 꺼내고, DTO로 맵핑하는건 DAO에서 하고,
DB에 데이터를 CRUD 할 때의 데이터 변환은 왜 service에서 하냐라는 의문점이 생길 수 있습니다.
저는 ResultSet
이 sql 패키지에 있기도 하고, 이 또한 DB접근 결과물이라고 생각합니다. Service에서는 DB 접근 결과물에 대해서를 알기보다는 애플리케이션 단에서의 데이터를 처리하는 역할이라고 생각해요. 즉, ResultSet
보다는 DTO겠죠? 그래서 이러한 코드를 작성했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아주 잘 생각정리를 잘 하셨네요 👍👍👍
public class BoardRegisterDao { | ||
|
||
private final String position; | ||
private final String turn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
도메인의 Board
는 private final Map<Position, Piece> chessBoard;
필드를 가지고 있는데 DB에 저장하자니 위와 같은 형태를 가지게 되서 이곳에서도 괴리감을 느끼셨을 수도 있을 것 같습니다.
근데 다시 생각해보면 현재 Board
는 체스판 그 자체로 생각했기 때문에 포지션과 해당 포지션의 기물에 대한 정보만을 갖고있는 것이고, 우르가 설계한 Board
DB는 체스게임 그 자체를 생각했기 때문에 포지션과 턴에 대한 정보를 갖고 있게 된 것이죠.
차라리 이 기회에 현재 Board
도메인의 개념을 단순히 체스판이 아닌 체스게임으로 생각하고 하시면 해당 도메인에도 turn 정보를 가지게 되고 DB와의 괴리감이 좁혀질 수도 있을 것 같네요.
그리고 현재 Board
DB에 집어넣을 때에 string으로 바꿔서 집어넣고 있는데 이것도 문제되지 않는다고 생각합니다.(우르가 문제되냐 묻진 않았지만....) 제 기준이지만, 실무에서 보통 엔티티에 컬렉션이 있는 경우 컨버터를 하나 만들어줘서 db에 넣기 직전, 빼고 엔티티에 매핑해주기 직전에 컨버팅을 해주는 컨버터를 등록해서 사용하고는 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저 또한 2번 방법이 낫다고 생각합니다. 경우에 따라 다를 수도 있을 것 같긴한데, 해당 체스미션에서는 2번 방법 좋다고 생각합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋ 파즈의 예상대로 살짝 괴리감을 느꼈습니다.
JPA에서의 가장 큰 장점은 객체와 테이블의 데이터 불일치 패러다임을 해결해준다잖아요? 그렇다면 결국 JPA에서 자동으로 해주는 것을 이번 미션에서는 제가 직접 컨버터나 매퍼를 사용하여 변환해주는 거니까요.(맞나요,,?)
차라리 이 기회에 현재 Board 도메인의 개념을 단순히 체스판이 아닌 체스게임으로 생각하고 하시면 해당 도메인에도 turn 정보를 가지게 되고 DB와의 괴리감이 좁혀질 수도 있을 것 같네요.
이 부분에서도 많은 공감을 했습니다.
요구 사항에서는 체스 방
을 만들어라 했는데, 저는 굳이? 라는 생각을 했습니다.
결국 "Board가 체스판
을 가지고 있으므로, Board
에 id
, turn
을 부여해서 체스 방으로 만들면 되지 않을까?" 처럼요.
하지만 여기서 괴리감을 느꼈습니다.
Board
는 private final Map<Position, Piece> chessBoard
를 가지면서 보드
의 행위와 상태를 관리하는 일급 컬렉션
이라고 생각합니다.
그래서 Board
에 turn
이나 id
를 넣게 되면 Board
일급 컬렉션의 의미가 퇴색되지 않을까?
BoardGame
을 만들어서 id
, turn
, board
를 가져야할까? 라는 생각이 들더라구요.
BoardGame
을 만들게 된다면 DB에는 Board 테이블
, BoardGame 테이블
이 존재해야할까?
아니면 BoardGame.java
에서 id
와 turn
을 빼오고, Board.java
의 Map<Position, Piece> chessBoard
을 빼와서 컨버팅 시켜서 하나의 Board 테이블
에 저장해야할까?
다들 가능한 방법이겠지만, 이러한 점이 제일 괴리감을 느껴요,, 그냥 제 선택일까요 파즈??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재의 Board
정보만을 토대로 체스 방
을 만든다면 굳이? 라는 말이 맞는 것 같습니다.
다만, 방 이름, 그리고 비밀번호 등의 컬럼이 추가된다는 가정 하에는 체스 방
이 있어도 될 것 같고 아마 그것 때문에 그렇게 명칭한게 아닐까 싶습니다. 이전에는 3,4 단계에 5단계까지 더 있었고 자바스크립트를 이용해서 ui 까지 구현을 했어야 했어서 설명의 잔재가 아닐까 싶네요.
도메인에 id가 들어가면서 데이터 중심적인 설계라고 느껴지고 일급 컬렉션의 의미를 잃는 것이 아닌가 라고 생각하는 것은 당연하구요, 트레이드 오프 인 것 같습니다. 그래서 보통 repository layer를 일급컬렉션으로 여기고 사용을 하게되죠. BoardRepository
가 있다면 해당 repository는 List<Board> boards
를 가지고 있는 repository다 라고 생각을 해도 무방하니까요. 굳이 Board
그 자체가 일급 컬렉션이여야 하는 이유는 없잖아요? 3,4 단계 미션을 진행하면서 요구사항이 추가가 되었고 그에 맞는 리팩토링을 하면서 turn을 추가하게 되고 어쩔 수 없이 더 이상 일급컬렉션이 아니라면 아니게 되는거겠죠. 이게 마음에 들지 않는다면, 현재 Board
네이밍을 ChessGameInfo
라고 두고 private final Map<Position, Piece> chessBoard;
필드를 따로 래핑하는 클래스를 만들면 괜찮지 않을까요? 클래스 이름만 변경을 해도 느낌이 확 다르게 다가오니까요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파즈 답변을 읽고, 다시 제 질문을 읽어보니 너무 일급 컬렉션
에 맞추려고 하는 듯합니다.
일급 컬렉션을 작성해놓고 나중에 추가적인 요구사항이 필요하면 능동적으로 변경될 필요가 있는데 그 틀에만 너무 잡혀있다는 생각이 드네요. 말씀대로 일급 컬렉션
을 그대로 유지하고 싶으면 파즈 말씀대로 Map만 가지고 있는 일급 컬렉션을 따로 만들고 Board에 추가해 줄 수 있을 것 같습니다.
System.out.println("현재 진행하고 있는 게임 번호들입니다."); | ||
|
||
final AllBoardSearchResponse allBoardSearchResponse = boardQueryService.searchAllBoards(); | ||
|
||
OutputView.printGameCandidates(allBoardSearchResponse.ids()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
게임 방 이름이나 방 생성인의 이름이 아니라 게임 번호들이면 결국엔 사용자가 본인의 게임 번호를 외우고 불러와야할텐데 그러면 진행되고 있는 게임 번호들을 모두 보여주는게 의미가 있나라는 생각이 듭니다.
그리고 먄약 게임 방이 몇천 몇만개면 이것 또한 문제가 있을 것 같네요. 차라리 해당 절차를 없애거나, 또는 일부만 불러오고 다른 커맨드를 입력해서 페이지 조회하듯이 할 수 있으면 좋을 것 같네요. 아니면 더 나아가서 방 이름을 추가해서 방 이름을 보여주던가 하는 식도 괜찮긴 하겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파즈가 말씀하신 거에 대해서 1000% 동감합니다.
저도 어디까지 기능을 구현해야하라는 고민이 있었습니다.
몇 천개, 몇 만개가 된다면 당연히 페이지네이션을 구현해야하고, id만을 주는게 의미가 있나라는 생각을 했었습니다.
현재 지금 기능에서 게임을 저장하고, "이 게임을 가져오는 방법을 구현했다"라고 보여드리기 위해서 코드를 작성했습니다.
findAll 같은 메서드는 거의 사용하지 않을거라 생각합니다 ㅋㅋㅋ 한번에 모든 데이터를 가져오는만큼 위험한 일이 없으니까요!!
final BoardSearchResponse boardSearchResponse = boardQueryService.searchBoard(loadedGameNumber); | ||
|
||
board = new Board(boardSearchResponse.chessBoard()); | ||
turn = Color.valueOf(boardSearchResponse.turn()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoardRegisterDao
에 남긴 리뷰를 적용하면 이쪽 부분이 훨 깔끔해질 수 있겠네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분을 깔끔하게 하기 위해서 + Board 자체가 id를 가져야하기 때문에
Board
에 인스턴스 필드로 id를 가지게 됐습니다. 그러면 결국 auto increment 로 인해 Board를 생성할 때, db에 저장돼야 id를 알 수 있습니다.
부득이하게 Board
에 아래와 같은 setter가 생기게 되는데 이러한 부분은 어쩔 수 없는 것이겠죠?
public void assignId(final Long id) {
this.id = id;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저런 메서드는 보통 setter로 치부하지 않습니다. 저것 또한 비즈니스 로직의 일부 이니까요.
setter를 지양하라는 것은 단순히 내부 필드에 바로 접근해서 할당을 하고 그 값 변화를 쉽게 되게끔 하지말라는 본 의미인데, 위의 메서드는 그런 개념이 아니라고 보기 때문에 괜찮다고 생각합니다.
import chess.domain.board.service.dto.BoardRegisterRequest; | ||
import chess.domain.board.service.mapper.BoardMapper; | ||
|
||
public class BoardCommandService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 체스미션에서 CQRS를 이용하게되면 어떤 장단이 있는지 의견 듣고 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
캬.. 파즈 이 리뷰를 기다렸습니다 !!!
먼저 제 생각을 전달드리기에 앞서, CQRS를 깊이 공부하진 않아서 정확하지 않는 지식이 있을 수도 있습니다!
왜 CQRS를 사용했는가?
단지 코드 상에서 Service를 Command와 Query로 분리했다고해서 CQRS를 이용하고 있다는 생각하진 않습니다.
왜냐하면 CQRS는 애플리케이션 코드에서 뿐만 아니라 각 책임에 알맞는 DB 설정 등 더 확장하게 사용하여야 CQRS를 이용하고 있다고 생각합니다.
그래서 저는 CQRS를 사용한다기 보다는 "상태를 변경시키고, 조회를 담당하고 있는 Service를 나누게 되면 더욱 가독성이 좋다" 라는 생각으로 적용했습니다.
그리고 제가 Command와 Query를 나누면서 DB를 따로 설정해보진 않았지만, 가독성이 높아진 것을 몸소 경험한 적이 있습니다.
대부분 애플리케이션이 커지게 되면 조회성 쿼리가 많아질텐데, 이를 하나의 Service에 넣어놓게 되면 코드를 찾을 때 힘들더라구요. 테스트 코드도 길어지구요. 그래서 한번 나눠보니 변경점을 찾는 부분부터, 테스트 분리까지 좋아져서 그 이후로 애용합니다.
굳이 적용해야해?
"Board에는 CRUD 밖에 없지 않아? 굳이 이렇게 분리해야돼?"라고 궁금해하실 수도 있습니다.
하지만 이러한 작은 애플리케이션에서도 분리를 하면서 테스트도 분리할 수 있고,
"조회성 쿼리는 QueryService에 있어" 등 코드 수정 시 코드를 찾는 시간과 변경 범위가 최소화 될 것 이라 생각합니다.
장단점 트레이드 오프
단점은 단지 클래스 하나가 늘어나고, Controller에 Service 의존성이 하나 더 늘어난다 라고 생각합니다.
하지만 제 적은 경험에서 클래스가 적은 것보다는 많은 것이 낫다 생각하고, 현재 애플리케이션에서는 Service 의존성이 하나 더 늘어난다고 해서 크게 영향이 있지 않을 거라 생각했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋㅋ 무수한 이모티콘의 향연이군요 칭찬의 의미시겠죠 ㅋㅋㅋ
혹시 제 설명에서 틀린 점이나 의문이 드는 부분은 없으실까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
괜찮은 것 같습니다~!
private static final String POSITION_AND_PIECE_DELIM = " : "; | ||
private static final String PIECE_DELIM = ", "; | ||
private static final String POSITION_DELIM = " "; | ||
private static final String PIECE_REGEX = ","; | ||
private static final String POSITION_REGEX = ":"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공백같은 것은 최대한 줄여서 컬럼 최대 길이를 줄일 것 같습니다. db 크기도 결국엔 돈이니까요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오,, 그렇네요
굳이 불필요한 공백을 사용할 필요가 없을 것 같아요.
"보기에 좋다"라는 생각을 했었는데 굳이 DB 데이터를 이쁘게 만들 필요가 없을 것 같습니다.
그래서 "K : 1 7, Q : 6, 1" -> "K:1 7,Q:6 1"로 변경시켰습니다.
- String 이 아닌 BoardRegisterRequest 으로 변경
안녕하세요 파즈 !! DB 셋팅다른 리뷰를 보니 DB를 셋팅해두었더라구요,, 그래서 저도 파즈가 더 쉽게 애플리케이션 동작하도록 셋팅 좀 해두었습니다.
DAO 수정DAO 를 잘못 이해해서 이번에 수정했습니다 !! 1차 리뷰 null 확인체스 1차 리뷰 복습하다가 제가 질문 드릴려고 정리해놨었는데 깜빡하고 안했더라구요,, 하하,, 파즈가 nullable 하다면 널 체크를 해주신다는 경향이 있는 것 같다고 말씀해주셨는데, 혹시 자바나 스프링 내부 코드를 보면 중간에 assertNull 이런 것들을 말씀하시는걸까요? |
저의 경우는 보통 컬렉션을 다루는 경우가 대부분이어서 stream 돌리면서 필터로 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 우르 피드백 반영하시고 재요청하신 내용 잘 봤습니다.
현재 원활하게 돌아가지 않는 상태인 것 같은데 해당 부분 수정하시면서 디테일만 조금 더 보강하면 될 것 같습니다! 코멘트도 다시 달았으니 확인해주시면 감사하겠습니다.
version: "3.9" | ||
services: | ||
db: | ||
image: mysql:8.0.28 | ||
platform: linux/amd64 # mac m1 | ||
restart: always | ||
ports: | ||
- "13306:3306" | ||
environment: | ||
MYSQL_ROOT_PASSWORD: root | ||
MYSQL_DATABASE: chess | ||
MYSQL_USER: user | ||
MYSQL_PASSWORD: password | ||
TZ: Asia/Seoul | ||
volumes: | ||
- ./db/mysql/data:/var/lib/mysql | ||
- ./db/mysql/config:/etc/mysql/conf.d | ||
- ./db/mysql/init:/docker-entrypoint-initdb.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안그래도 ddl 올려달라고 말하는걸 까먹고 못말했는데 덕분에 편하게 했습니다 👍
end(board); | ||
return; | ||
} | ||
|
||
turn = movePiece(board, turn, moveCommand, startCommand); | ||
if (Command.isStatus(startCommand)) { | ||
status(board); | ||
return; | ||
} | ||
|
||
final Turn beforeTurn = board.turn(); | ||
|
||
if (Command.isMove(startCommand)) { | ||
move(board, moveCommands); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 때문에 게임 진행이 안되고 있네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
으악,, 죄송합니다 !! 바로 수정하겠습니다
return null; | ||
} | ||
|
||
public void modifyById(final BoardModifyRequest boardModifyRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예외가 터지면 저장도 못하고 그대로 끝이 나게되는거죠?
저장을 현재 end와 status 때에만 호출하고 있는데,
status라는 기능이 있는지 InputView에서 출력하고 있지 않아서 알 수가 없고, end 시에도 DB 저장이 되는지 알 수가 없는 상태입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하,, 그렇군요 전혀 사용자를 고려하지 않은 것 같습니다. status 를 추가하고, end, status를 입력하면 게임을 저장할 수 있는 것을 명시해두도록 하겠습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 색이 다른 말을 선택하면 다시 선택하는게 아니라 어플리케이션이 바로 종료가 되는데요,
start 하고 한 30수를 진행했는데 다른 말을 선택해서 바로 꺼져서 저장이 안되면 굉장히 허무할 것 같습니다.
예외가 터져서 어플리케이션이 종료되기 전에 현재 board를 save 하거나 하는 디테일로직이 있었으면 더 좋았을 것 같네요!
@Override | ||
public boolean equals(final Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
final Turn turn = (Turn) o; | ||
return color == turn.color; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(color); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드에서만 사용되고 있는 것 같은데 해당 테스트 코드에서도 .color()
를 이용해서 비교할 수 있을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color()
가 있으니 () -> assertEquals(turn.color(), savedTurn.color())
와 같이 하면 되지 않을까 싶었습니다!
public class BoardModifyRequest { | ||
|
||
private final Long id; | ||
private final String position; | ||
private final String turn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
총 3개의 dto 클래스가 있는데 이 정도 유사성이면 사실 그냥 BoardDto
로 두고 공용으로 사용하는 것이 프로젝트의 규모가 커졌을 때에 재사용성이 좋긴합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 부분에 대해서 매번 고민을 하고 있습니다.
거의 비슷한 인스턴스 필드를 가지고 있으면 그냥 BoardDto
로 공통으로 사용하는게 좋지 않을까? 라는 생각이 들구요.
"BoardDto
라는 네이밍 자체가 너무 추상적이지 않나"라는 생각도 듭니다.
BoardSearchResponse
, BoardRegisterRequest
등 DTO의 이름만 봐도 어느 곳에서 사용되는지 알 수 있다는 확실한 장점이 있지만, 클래스가 너무 많아진다는 단점이 생기기도 하구요.
제 나름대로의 고민에 대한 결과는 클래스가 적은 것보다는 많은게 낫다라는 생각이었고, DTO라는게 대부분 일회성 데이터 전달 객체이기 때문에 굳이 공통으로 사용해야할 필요가 있을까에 대해서 의문이 들더라구요.
제가 현업에서 일을 해본 적이 없어서 많은 경험을 하진 않아서 파즈의 고견을 듣고 싶어요.
프로젝트의 규모가 커지면 공통으로 사용해서 재사용성을 높일 수 있다고 하는데, 대부분 현업에서의 응답, 요청 값들이 대부분 같을 수가 없다고 생각해요. 그래서 공통으로 사용할 수 있는 부분이 많을까 ? 라는 의문이 듭니다.
그리고 두번째로는
// BoardRegisterRequest
private final String position;
private final String turn;
// BoardSearchResponse
// BoardModifyRequest
private final Long id;
private final String position;
private final String turn;
이렇게 사용하고 있는데
3개의 DTO를 하나의 공통 DTO로 만들게 된다면 id는 null 인 채로 들어가게 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 경우가 좀 다른 부분이라 코멘트를 남길까 말까 고민을 하다가 남겼었습니다.
일단 modify의 경우 jpa를 사용하다 보니깐 id로 entity를 검색하고 수정해서 더티체킹으로 modify 시켜주니깐 dto가 필요없고 단순히 findById 메서드에 id를 넘겨주기만 하니깐 현 미션에서의 modifyRequest가 사라질 것이고요,
create의 경우에도 jpa를 사용하다 보니깐 dto를 보내지는 않죠. 조회의 경우에도... entity를 조회하고 그것들을 조합해서 responseDto를 만들긴하니깐 repository에서 service로 dto를 보내는 경우가 거의 없습니다.
service 계층에서 따로 dto를 조합하는 경우는 생기기는 합니다. 이 경우는 여러 엔티티의 정보가 필요한 dto의 경우여서 service 계층에서 조합을 해주는 경우입니다.
재사용 하는 경우는 아 이거 정말 케이스가 다르긴한데, a 라는 모듈에서 다른 모듈의 api를 호출한다고 가정합시다. api 호출은 feignClient 라는 것을 사용해서 호출을 하고 있습니다. b 모듈에서 ProductDto 라는 필드가 30개 박혀있는 dto를 return 하고 있는 상황입니다.
b 라는 모듈에서 제가 해당 api를 호출을 해서 써야합니다. 그러려고 보니깐 팀원분이 이미 해당 api를 호출을해서 받는 메서드를 정의를 해놨습니다. 그 팀원분은 필드 30개를 모두 사용하는 것이 아니라 5개만 필요하기 때문에 a 모듈에 해당 b 모듈의 api 메서드 코드를 작성할 때에 필드 5개를 받는 dto를 생성해둡니다.(이거는 feign을 사용안해보셨으면 코드 상상이 안될 수도 있을 것 같네요... 이 부분은 그런갑다 하셔도 됩니다)
아무튼 저는 마침 다른 팀원분이 필요로 하는 필드 5개 중 3개가 필요한 상황입니다. 이 때에 굳이 필드3개의 dto를 만들고 feign 메서드에 추가할 필요없이 팀원분이 만들어두신 것을 호출하고 그곳에서 필요한 3개만 가져다가 써도 된다는 것이죠. 이런 경우에서의 재사용이 있습니다.
이제 조회에서의 재사용을 한 번 말해보겠습니다.
class ProductCriteria {
long productId;
long merchantId;
long categoryId;
boolean aFilter;
boolean bFilter;
}
검색을 하는 경우입니다. 검색을 할 때에 여러 필터를 걸 수도 있고 productId만 입력해서 검색을 할 수도 있고 productId와 merchantId 둘 다 입력해서 검색을 할 수도 있습니다.
이 dto는 controller 에서 가장 처음 reqeustBody로 받아들여질 것인데요. 이런 모든 검색의 조건마다 dto를 만들 수 있을까요? 만들 수는 있겠지만 비효율적이겠죠. 이런 경우에는 repository 쪽으로 바로 dto를 보냅니다. 그리고 그곳에서 criteria 나 querydsl을 이용해서 동적으로 판단하여 조회를 하게되겠죠. 이것도 어떻게보면 dto의 재사용이라고 볼 수도 있을 것 같네요.
또 여러 케이스가 있긴할텐데 일단 이 정도로 마무리 하도록 하겠습니다.
사실 이런 간단한 체스 미션에서는 생각하시는대로 하시면 되긴합니다. 어차피 현업가면 보통 기존의 코드 컨벤션이나 암묵적인 룰을 맞춰서 가기 때문에 크게 고민하실 일은 없으실겁니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
물론 null이 들어가는 경우도 있을 것입니다. 그것을 판단해서 조회 쿼리가 완성됩니다.
안녕하세요 파즈,, !! 마지막 리뷰 요청이네요,,!! |
turn = movePiece(board, turn, moveCommand, startCommand); | ||
if (Command.isStatus(startCommand)) { | ||
status(board); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status 를 봐도 게임이 꺼지고 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우르 3,4단계 수고하셨습니다. 남기신 코멘트에 다시 코멘트 달았고 추가적으로 코드 코멘트 남겼습니다.
확인해주시면 감사하겠습니다. 룰에 따라 이만 머지하도록 하겠습니다. 수고하셨어요.
안녕하세요 파즈!!
저번 리뷰 때 말씀해주신 것 모두 수정하여 pr 제출합니다.
바쁘신 와중에도 리뷰해주셔서 감사합니다 :)
이번 리뷰 잘 부탁드립니다!!
3단계
King이 잡혔을 때 게임 종료
현재 남아있는 말에 대한 점수 구하기
3단계를 진행하면서 다시 한번 일급 컬렉션의 필요성을 느낄 수 있었습니다.
열마다의 일급 컬렉션을 사용하고, 해당 일급 컬렉션에서 계산을 진행했습니다. 그리고 열들을 가지고 있는 BoardScore에서 총 점수를 계산하여 반환합니다.
이렇게 작성하니 책임과 역할을 적절하게 나눌 수 있었고, 테스트 하기도 원활했습니다.
또한, King 이 잡혔을 때의 구현도 BoardSearch 라는 객체가 체스들의 개수를 가지고 있어서 쉽게 구현할 수 있었습니다.
아래 이미지는 제가 미션을 진행하면서 작성했던 다이어그램입니다.
4단계
DB 적용
DB를 적용하니 스프링을 할 때와 구조가 매우 비슷하게 흘러가게 됐습니다.
처음 개발 공부를 할 때부터 스프링을 하다보니, 항상 똑같은 구조를 가지게 코드를 작성하더라구요.
Controller - Service -Repository
그리고 DM 에서 파즈와 얘기를 나눴듯이 저만의 객체지향 세계는 domain 에서 펼치기로 했습니다.
결국 이렇게 계층이 나뉜 것도 어떻게 보면 책임과 역할에 따라서 나뉘게 된거니까요.
도메인이 가장 중요하기 때문에, 레벨1에서는 그 도메인을 가장 잘 다루는 방법을 배웠다고 생각합니다.
DAO ? Repository?
이번 4단계 미션을 하면서 가장 헷갈렸던 부분입니다.
그래서 DAO와 Repository는 뭘까에 대해 검색을 해보니, 알 수 없는 단어들이 많았습니다.
추상화 정도, 누가 더 도메인에 가깝다, 누군 update가 있다 없다, Repository 는 DDD로부터 파생됐다 등 다양한 글과 과거 영호님이 작성하셨던 글을 봤는데 결론은 이해하지 말자였습니다.
개념은 개념대로 두고, 이런 것에 선을 나누는데 너무 많은 시간을 사용할 필요가 있을까라는 생각을 했어요.
그래서 저는 DAO는 Data Access Object로 현재 테이블에 저장된 데이터 타입과 자바 애플리케이션에서의 데이터의 타입이 다르기 때문에 그 불일치를 해결해주는 객체라고 생각했습니다.
그리고 Repository는 실제 DB에 요청을 보내는 쿼리들이 존재하고 있습니다.
Controller
Controller를 이쁘게 만들고 싶었는데,, 도저히 방법이 떠오르지 않더라구요.
크루들과 얘기를 나누면서 커맨드 패턴, 상태 패턴 등 다양한 것들이 나오면서 코드를 줄일 수 있다고 하는데, 제 생각에는 if 문이 가장 직관적이었어요.
크루들이 말한 패턴들을 읽어보고 조금 적용해보니, 코드 수는 줄일 수 있더라도 반비례로 상대방이 더 이해하기 어렵다고 생각했어요.
그래서 결국 if 를 사용하고 있습니다.
만약 파즈가 해당 코드를 보시고 개선점이 보이신다면 말씀해주시면 적극 반영하겠습니다!!
테이블 설계
이번에 테이블 설계에서 두 가지 고민을 했습니다.
1번 방법은 정규화를 통해서 Piece를 나눠서 확장성이 더 좋다고 생각하지만, Board 1개가 생성될 때마다 32개의 튜플들이 생긴다는 단점이 있습니다.
2번 방법은 1번을 비정규화하여 하나의 튜플에 위치를 모두 저장하게 됩니다. 1번 방법보다 저장되는 튜플이 적어지지만 그만큼 varchar 의 길이가 매우 길어집니다. 그리고 확장성도 떨어지게 되구요.
이러한 이유로 비교했을 때 수정에 관해서는 1번, 2번 방법이 모두 비등합니다.
하지만 생성과 조회에서 2번 방법이 더욱 낫다고 생각하여 택하게 됐습니다 !
긴 글 읽어주셔서 감사합니다.